Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

sessionctx: record if a query hits plan cache in slow log #17088

Merged
merged 5 commits into from
May 12, 2020
Merged

sessionctx: record if a query hits plan cache in slow log #17088

merged 5 commits into from
May 12, 2020

Conversation

eurekaka
Copy link
Contributor

What problem does this PR solve?

Issue Number: close #16197

Problem Summary:

Not a problem, this is an enhancement for diagnose purpose.

What is changed and how it works?

What's Changed:

Add new Plan_from_cache field in slow log.

Related changes

  • PR to update pingcap/docs/pingcap/docs-cn:
  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test

Side effects

N/A

Release note

  • record if a query hits plan cache in slow log

@eurekaka eurekaka added type/enhancement The issue or PR belongs to an enhancement. component/session needs-cherry-pick-4.0 labels May 11, 2020
@eurekaka eurekaka requested review from a team as code owners May 11, 2020 07:25
@ghost ghost requested review from wshwsh12 and winoros and removed request for a team May 11, 2020 07:25
@github-actions github-actions bot added the sig/execution SIG execution label May 11, 2020
@eurekaka eurekaka requested a review from zz-jason May 11, 2020 07:52
zz-jason
zz-jason reviewed May 11, 2020
View reviewed changes
Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please also update the slow_query table.

@eurekaka eurekaka requested a review from danmay319 May 11, 2020 08:52
@github-actions github-actions bot added the sig/sql-infra SIG: SQL Infra label May 11, 2020
danmay319
danmay319 reviewed May 11, 2020
View reviewed changes
planner/core/common_plans.go
@@ -350,10 +350,6 @@ func (e *Execute) getPhysicalPlan(ctx context.Context, sctx sessionctx.Context,
e.Plan = p
_, isTableDual := p.(*PhysicalTableDual)
if !isTableDual && prepared.UseCache {
err = e.setFoundInPlanCache(sctx, true)
Copy link
Contributor

@danmay319 danmay319 May 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should have been removed by https://github.com/pingcap/tidb/pull/16594/files, wonder why it appears again. 🤦

@eurekaka eurekaka removed the sig/sql-infra SIG: SQL Infra label May 11, 2020
danmay319
danmay319 reviewed May 11, 2020
View reviewed changes
executor/adapter.go
@@ -843,6 +843,7 @@ func (a *ExecStmt) LogSlowQuery(txnTS uint64, succ bool, hasMoreResults bool) {
PlanDigest: planDigest,
Prepared: a.isPreparedStmt,
HasMoreResults: hasMoreResults,
PlanFromCache: sessVars.FoundInPlanCache,
Copy link
Contributor

@danmay319 danmay319 May 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FoundInPlanCache indicates "if the current executing statement is from plan cache", while PrevFoundInPlanCache indicates "if the last executing statement is from plan cache". Currently, when we execute select last_plan_from_cache, the value of PrevFoundInPlanCache is shown as result. So I wonder which value is needed here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LogSlowQuery is called before finishing execution of current query, so we should use FoundInPlanCache I think.

@github-actions github-actions bot added the sig/sql-infra SIG: SQL Infra label May 11, 2020
danmay319
danmay319 approved these changes May 11, 2020
View reviewed changes
Copy link
Contributor

@danmay319 danmay319 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@danmay319 danmay319 added the status/LGT1 Indicates that a PR has LGTM 1. label May 11, 2020
crazycs520
crazycs520 approved these changes May 12, 2020
View reviewed changes
Copy link
Contributor

@crazycs520 crazycs520 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@codecov
Copy link

codecov bot commented May 12, 2020

Codecov Report

Merging #17088 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #17088   +/-   ##
===========================================
  Coverage   80.1299%   80.1299%           
===========================================
  Files           510        510           
  Lines        140226     140226           
===========================================
  Hits         112363     112363           
  Misses        18868      18868           
  Partials       8995       8995

@zz-jason zz-jason added status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels May 12, 2020
@sre-bot
Copy link
Contributor

sre-bot commented May 12, 2020

Your auto merge job has been accepted, waiting for:

  • 16078

@sre-bot
Copy link
Contributor

sre-bot commented May 12, 2020

/run-all-tests

@sre-bot sre-bot merged commit cfec137 into pingcap:master May 12, 2020
@eurekaka eurekaka deleted the slow_log branch May 12, 2020 07:49
sre-bot pushed a commit to sre-bot/tidb that referenced this pull request May 12, 2020
@eurekaka @sre-bot
sessionctx: record if a query hits plan cache in slow log (pingcap#17088
93f2d24 
)
@sre-bot sre-bot mentioned this pull request May 12, 2020
Merged
@sre-bot
Copy link
Contributor

sre-bot commented May 12, 2020

cherry pick to release-4.0 in PR #17121

sre-bot added a commit that referenced this pull request May 12, 2020
@sre-bot
sessionctx: record if a query hits plan cache in slow log (#17088) (#…
f5146b0 
…17121)
@Reminiscent Reminiscent mentioned this pull request Jun 15, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zz-jason zz-jason zz-jason left review comments

@crazycs520 crazycs520 crazycs520 approved these changes

@danmay319 danmay319 danmay319 approved these changes

Assignees
No one assigned
Labels
component/session sig/execution SIG execution sig/sql-infra SIG: SQL Infra status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

record whether a statement hit plan cache in slow log
5 participants
@eurekaka @sre-bot @zz-jason @danmay319 @crazycs520